-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add LoadMap service to map_server #1303
Add LoadMap service to map_server #1303
Conversation
I know you're probably just back from ROSCon, but really you just need to publish the updated map and this can just be merged. |
I'm going to do the 3 things in the 'to do' list above: fail codes, unit test and docs before I mark this ready for review |
I don't understand this Circle CI fail: Config Processing Error (Don't rerun)00:00
@crdelsey or @ruffsl if you know what this means please help |
It may be draft PR related. |
Looking at the link for the CI, it seems that draft PRs are executed under the PR owner's org: Note the org is listed as See my #990 (comment) here. |
Closing - will re-open to see if CI works |
I tried -
None of those seem to work, I'm getting the same build error |
Did you try rebasing? |
Also, are you ready now for a review? |
93e9186
to
715775c
Compare
Just rebased and re-pushed. That seems to have re-triggered circleci. Fingers are crossed. Assuming that passes, it's ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small stuff
I think I've addressed all the feedback. Please take another look. |
nav2_msgs/srv/LoadMap.srv
Outdated
uint8 TYPE_URL=1 | ||
|
||
# Type of map resource | ||
uint8 type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to coordinate with my ROS 1 PR (ros/common_msgs#152) to make bridging in the future more straight forward.
A path to the file can also be represented as a URL. I think doing something similar to what I've done in ros/common_msgs#152 leaves the door open to other locator types without having to modify the message definition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
jacob, how do you plan to get the YAML file from the URL? Are you planning to use curl
or is there an easier way to load a YAML file directly from a URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't thought about it too much. For the file URL, curl
might work. As a first iteration, it might also be just as easy to write (or find) a simple URL parser to check if the prefix of the URL is file://
and then take the rest of the URL as the path to the YAML file.
To load a YAML from a package directory (eg. package://
) we'll need URL parser anyways to get the package name and relative path, and then we can use the ament API to locate the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, we should probably use the existing resource_retriever for the job. It it a curl-like tool that also supports package://
:)
I forgot about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that's very helpful. I wasn't aware of resource_retriever.
I'll have to re-write part of this PR to use that and the message definition you created.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, coordinate with Jacob
So @SteveMacenski and @crdelsey - If I continue with the service and What are your thoughts on this? |
# URL of map resource | ||
# Can be an absolute path to a file: file:///path/to/maps/floor1.yaml | ||
# Or, relative to a ROS package: package://my_ros_package/maps/floor2.yaml | ||
string map_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also still take a relative or full path by default if no [thing]://
exists for backwards compatibility.
That would be a huge hindrance for anyone moving to ROS2 to have to change all their maps. Am I correct in reading that this (https://github.com/ros-planning/navigation2/pull/1303/files#diff-e5c96c9122118a911c895ffa5dbd7b00R111) does that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding was that this feature doesn't exist in ROS 1 (yet), so what do we have to be backwards compatible with?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yes. Sorry I pointed out this line because of the instructions for [thing]://
not the .srv
file in particular. I wanted to make sure that in all cases that the map.yaml
file that contains the image: testmap.png
field will accept image: testmap.png
as well as image: file:///path/to/testmap.png
and image: package://my_map_package/maps/testmap.png
. That way we're backwards compatible with relative or absolute paths but then now we can use the package
path (which will be super nice)
Your question made me realize maybe I'm misintepreting this - this is just for the loading from remote servers of the... yaml file? How does the png/jpeg/pgm get picked up? It reads to me like the read_to_temp
only grabs the yaml
file, not the image
file inside of it too.
I think this PR has gotten to complex for at least me. I think the load map service should be introduced before we start messing around with remote files or changing how we formulate the paths. It seems that if you're going to accept [thing]://
anywhere, it should be in the image: [image-name].[image-extension]
, or at least doing that as well when you get the yaml
file from remote, getting its matched image
file too
I think right now this only grabs the remote yaml file and just assumes the image inside the yaml is valid? That's not a good assumption. I don't think we should introduce that feature in this PR, it brings up too many blocking questions. For instance, what does an absolute path now mean when you pulled a file from the server, or relative paths when you're rewriting the path by downloading it. package://
is the only thing that would have that make sense but assumes the same packages installed and the same version of those packages are installed. This is much larger question I don't think there's a canned prepared answer to fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Steve, you bring up good points and I think I've addressed them in this PR (which is why it took longer than I hoped).
- if "package", "http" or "file" are not the first words in the path given, resource_retriever is not used, instead it is treated as an absolute local file path (same as before, so we stay compatible with existing files)
- if any of those are given, it will fetch the yaml file, parse it, and then treat the image path as a relative path to the yaml file. So if the yaml is at "http://my.server/file.yaml" and the image name given is "file.png" it will be treated as "http://my.server/file.png".
I didn't test the "package" version, but I did test the "http", "file" and default. I've added unit tests for all of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For bullet 1: only absolute file path? Originally it worked with relative paths and from looking at the code I don’t see in particular why relative would now fail.
For bullet 2: doesn’t that imply it ignores the yaml key for the image name and just assumes it has the same name in the same directory? That’s not right... I also only see where you download the remote yaml, where’s the image retrieval?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would disagree, they have to be changable. Just because the yaml file is being retrieved locally or on a server doesn't mean that the image file will be described the same way. Ei, it shouldn't be on the resource itself to know where the caller is (remote, local, absolute, relative). The yaml doesn't have to be anywhere near the image file, or like in your example 2 its gotten from remote but the image key is by a package
path and what happens if that package isn't installed on the caller's machine?
Also we need to be caching these maps, we cannot assume that for every time the server is called that its going to have internet access, if its called it should be downloaded and stored locally so that it can be retrieved locally before trying to call out to a remote server. You don't want to be trying to change maps in the middle of a warehouse in a dead zone and then sit there forever because you have no connection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has alot of edge cases and problems needing to be thought about more. I think all the webstuff should be stripped out of this PR, its trying to change too much at once. The original ticket was just to have a change map service exposed. It didn't have anything to do with changing how we represent image:
yaml keys or retrieving remote resources. That seems to be a whole other conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just because the yaml file is being retrieved locally or on a server doesn't mean that the image file will be described the same way
True. I was suggesting we make this assumption for simplicity. As I suggested, we can instead use a URL for the image key, then it doesn't matter where the yaml was retrieved from. There are still edge cases if we also allow regular paths (e.g. do we interpret them as being local or relative to the yaml). Perhaps, removing any assumptions about locality between the yaml and image makes things easier.
in your example 2 its gotten from remote but the image key is by a package path and what happens if that package isn't installed on the caller's machine?
My second example doesn't show the image key is a package path. It is a path relative to map.yaml.
Also we need to be caching these maps, we cannot assume that for every time the server is called that its going to have internet access, if its called it should be downloaded and stored locally so that it can be retrieved locally before trying to call out to a remote server.
This sounds like an additional feature to be added to the map server and beside the question of what the semantics are for the URL and image key.
This has alot of edge cases and problems needing to be thought about more. I think all the webstuff should be stripped out of this PR, its trying to change too much at once
+1
It's good that we're having this discussion! But, we should probably move it to ros/common_msgs#152. I agree that for the first pass, this PR should be an incremental change (e.g. exposing the service). I just wanted to make sure we define the service interface appropriately for the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the srv type, I have nothing to add, I think its fine and I wouldn't change anything about it. I think the question is in the behavior of use.
This sounds like an additional feature to be added to the map server and beside the question of what the semantics are for the URL and image key.
I don't think it is, I think its a precondition to anything with non-local retrieval. We cannot have a situation where we're dependent on the internet to continuously download the same 5 assets we downloaded yesterday.
I think you and I agree now that this should probably be part of another discussion though, whether its on common msgs or in a ticket here (common_msgs is as good a place as any).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For discussion about the behavior, it's probably best to create a ticket in this repo since I think it specific to the nav2_map_server.
f38818c
to
d389d81
Compare
OK, I backed out the resource retriever support since that usage seems to be unclear. Let's get this in for simple local files only, and I'll open a second PR with those changes and we can discuss separately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not throw? I don't like servers going down when there's other actions that can be taken to correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
d389d81
to
1f52a51
Compare
Basic Info
Description of contribution in a few bullet points
Added a "LoadMap" service, which accepts a filename and loads the file into the server
Still To Do:
Return fail codes (marked as TODO in code)
Create a unit test
Update documentation
Future work that may be required in bullet points